Ignore disconnected status while changing region #16285
Conversation
simonhong
force-pushed
the
brave_vpn_change_region_status
branch
4 times, most recently
from
4647ca6
to
0749bf4
Compare
simonhong
force-pushed
the
brave_vpn_change_region_status
branch
from
0749bf4
to
e8330bb
Compare
When user changes region while connected, status should be connecting till region change completes.
spylogsster
force-pushed
the
brave_vpn_change_region_status
branch
from
e8330bb
to
75920b0
Compare
|
|
| // noti again. So, ignore second one. | ||
| // On exception - we allow from connecting to disconnected in canceling | ||
| // scenario. | ||
| // Ignore disconnected state while connecting is in-progress. |
There was a problem hiding this comment.
These updates to comments lose the context for WHEN this can happen- which is when person is changing regions.
Can we add that back in?
// When user connects to different region while connected,
// we disconnect current connection and connect to newly selected
// region.
There was a problem hiding this comment.
wait- I missed the actual change to the logic 😅 You're correct to delete this, sorry!
| @@ -585,14 +585,6 @@ void BraveVpnService::LoadPurchasedState(const std::string& domain) { | |||
| return; | |||
| } | |||
|
|
|||
| #if !BUILDFLAG(IS_ANDROID) | |||
| if (!IsNetworkAvailable()) { | |||
There was a problem hiding this comment.
I think removing this code is great 👍 LoadPurchasedState is called when person is clicking on VPN button... and if they didn't have network connectivity (briefly, for example), this code would be letting the connection go into FAILED (when no connect ever was made by user).
Nice! 😄 👍
There was a problem hiding this comment.
As we cache expiration date, browser will enter to purchased state even network is disabled.
Then, user can get connect failure when user try to connect.
Or it's already expired, browser will ask to refresh skus credential and skus service will make use not purchased state.
So we could get proper state w/o this early checking.
Ignore disconnected status while changing region
Verified
|
| Brave | 1.48.57 Chromium: 108.0.5359.128 (Official Build) nightly (64-bit) |
|---|---|
| Revision | 1cd27afdb8e5d057070c0961e04c490d2aca1aa0-refs/branch-heads/5359@{#1185} |
| OS | Windows 10 Version 22H2 (Build 19045.2364) |
Steps:
- purchased, set up, and connected to
BraveVPNviaaccount.bravesoftware.com - confirmed I was connected to the
USA (West)server/region, by default - clicked on the
VPNbutton on the browser toolbar - selected
Australiaand clickedConnect - confirmed no disconnects or other errors (but the DoH warning dialog*, as expected), while changing connections
- selected
Spain - confirmed no disconnects or other errors, same as above
BraveVPN connected to USA (West) |
Australia |
Spain |
* DoH warning dialog |
|---|---|---|---|
 |
 |
 |
 |
fix brave/brave-browser#27267
fix brave/brave-browser#27196
When user changes region while connected, status should be connecting till region change completes.
Also removed network status checking with
IsNetworkAvailable().Instead of using
IsNetworkAvailable(), vpn service will get actual state by issuing network request.Also do load & save prefs twice when creating vpn entry on macOS to fix connect failure when there is no os vpn entry on macOS
Resolves
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run lint,npm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: